Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor for the meta functions #1332

Closed
wants to merge 26 commits into from

Conversation

SteveBronder
Copy link
Collaborator

@SteveBronder SteveBronder commented Sep 1, 2019

Summary

This refactors the metaprogramming structs and aliases to be a bit more consistent in their definitions and use more of the C++14 standard library methods. A few of the highlights are

  1. Instead of using partial specializations that required explicit specializations for const and & types, we use SFINAE and enable_if_t<std::decay_t<T>> to turn on and off the specializations for things like is_vector

  2. Edit: scalar_type removes const but leaves refs and pointers which is what the original implementation did

  3. scalar_seq_view now uses SFINAE to make sure types use the correct specialization.

  4. Deletes ad_promotable in fwd and rev. Instead we use the std::is_convertible check that the types are implicitly convertible.

  5. is_fvar depended on knowing fvar, but the .hpp of fvar depends on the meta folder which defined fvar! So I made fvar.hpp not depend on the meta programming.

  6. new structs is_std_vector and is_eigen to check if something is a standard vector or derived from EigenBase

  7. is_vector now checks if the eigen vector has rows or columns at compile time equal to 1.

  8. Moved a number of metaprogramming from arr and mat into scale when it could be instantiated there with specializations for vector and eigen left in arr and mat

  9. Cleaned up some of the docs

  10. The new struct bool_constant, which is just std::integral_constant<bool, bool B>, is inherited by most of the structs that are used to check for types at compile time.

  11. Some of the tests checked types for std::vector<const double> or std::vector<double&> which is illegal under C++11 (types in std::vectors have to be movable and erasable so these were changed to look for pointers.

  12. structs containing a type now use using instead of typedef

13. constexpr helper variable templates so we can call is_vector_v<T> instead of is_vector<T>::value (THX WINDOWS)

Tests

The only thing I still need to do is update the tests for is_std_vector and is_eigen

Side Effects

Should be none

Checklist

  • Math issue Update internals to use more modern c++ #1308

  • Copyright holder: Steve Bronder

  • the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)
  • (YAS) the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

library methods or coalesce them to be more consistent
@SteveBronder SteveBronder changed the title This refactors for the meta functions Refactor for the meta functions Sep 1, 2019
@rok-cesnovar
Copy link
Member

This seems like it will go green. If you dont have anyone else in mind for review feel free to assign me. I have enough experience with the traits in Stan Math that I feel comfortable with this.

@SteveBronder
Copy link
Collaborator Author

Awesome ty! Yeah I'll assign you if you don't mind

T, std::enable_if_t<std::is_const<T>::value || std::is_pointer<T>::value>> {
using type = typename scalar_type_base<
std::remove_pointer_t<std::remove_cv_t<T>>>::type;
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bob-carpenter @syclik this is probs the only serious change to point out. I'm assuming the intent of scalar_type is to get the underlying value so this also cycles through pointers. Does this seem like it's right?

@stan-buildbot
Copy link
Contributor

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.98)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 1.01)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 0.97)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 1.01)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 0.89)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 1.0)
(compilation, 1.03)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 1.01)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 0.98)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 1.0)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 0.97)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 1.0)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 1.0)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 0.99)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 0.93)
Result: 0.98347832807
Commit hash: f4b9a7d

@SteveBronder
Copy link
Collaborator Author

SteveBronder commented Sep 3, 2019

@serban-nicusor-toptal Do you know what's up with this error?

Edit: Nvm we are good!

C:\Rtools\mingw_64\bin\ar.exe: unable to rename 'lib/sundials_4.1.0/lib/libsundials_nvecserial.a'; reason: Permission denied

@stan-buildbot
Copy link
Contributor

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.97)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 0.99)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 0.97)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 0.98)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 0.91)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 0.99)
(compilation, 1.03)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 0.99)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 0.99)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 1.01)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 0.98)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 1.0)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 0.99)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 1.0)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 0.94)
Result: 0.98249725256
Commit hash: e17d59c

@serban-nicusor-toptal
Copy link
Contributor

Glad this was solved, I will look into it to see why this happened.

@rok-cesnovar
Copy link
Member

@SteveBronder will have a look this evening or tomorrow morning.

@stan-buildbot
Copy link
Contributor

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.96)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 1.03)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 0.97)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 1.06)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 0.91)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 1.01)
(compilation, 1.02)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 1.01)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 0.99)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 1.0)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 0.98)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 0.99)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 0.99)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 1.04)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 0.96)
Result: 0.99432279941
Commit hash: d146a0d

@syclik
Copy link
Member

syclik commented Sep 4, 2019

Awesome! I glanced at it and it looks rad. (@rok-cesnovar, stay on as the reviewer please. Thank you!)

@rok-cesnovar
Copy link
Member

Sorry Steve, I havent gotten to this yet. Will do tmrw.

@SteveBronder
Copy link
Collaborator Author

No worries!

Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic stuff Steve! Some minor stuff on the tests and one or two comments.

test/unit/math/prim/scal/meta/is_eigen_test.cpp Outdated Show resolved Hide resolved
stan/math/prim/scal/prob/inv_gamma_lpdf.hpp Show resolved Hide resolved
stan/math/prim/scal/meta/value_type.hpp Show resolved Hide resolved
stan/math/prim/scal/meta/promote_args.hpp Show resolved Hide resolved
stan/math/prim/scal/meta/ad_promotable.hpp Show resolved Hide resolved
…includes from from scal and arr tests. Fixed docs for is_var.hpp
@serban-nicusor-toptal serban-nicusor-toptal added this to the 3.0.0 milestone Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants